-
Notifications
You must be signed in to change notification settings - Fork 227
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add no-mocks-import rule #246
Conversation
Generated by 🚫 dangerJS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hell yeah, this is awesome! Just update the table in the readme, and this should be good to go
9d4b26d
to
ca843e3
Compare
Updated readme, thanks Danger 😅 |
I assume the changelog file doesn't need to be updated? It does not seem very complete 😅 |
Nope - it's supposed to be generated, but it messed something up so I disabled it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bah, forgot to submit the review
index.js
Outdated
@@ -27,6 +27,7 @@ module.exports = { | |||
'jest/no-focused-tests': 'error', | |||
'jest/no-identical-title': 'error', | |||
'jest/no-jest-import': 'error', | |||
'jest/no-mocks-import': 'error', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth the break?
If yes, we should look at all rules and land a single pr updating the recommended rules
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's always an error so it belongs in recommended? For linting I'd even say just release new default rules when they're ready, it's better to have many small majors than one big major where people see hundreds of errors after upgrading and decide to just disable the new rules because they're annoying ^^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's still a breaking change. And it might not always be an error even though I agree it's not a good idea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So to be clear, are you suggesting not to add it to recommended or to add it to recommended at a later point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add it to recommended at a later point :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. We can leave the line adding it to recommended commented out to keep track. Are there any other rules that you'd want to go into recommended in the next major?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, a comment is a good idea.
I'd like to activate the ones for using circus (banning jasmine globals) at least
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it. Didn't add a commented out one for jasmine globals because it's already there (just as warn
)
BREAKING CHANGE: Add no-mocks-import to recommended rules
ca843e3
to
5fc1c98
Compare
The publish failed for some reason... https://travis-ci.org/jest-community/eslint-plugin-jest/jobs/523169312#L857 Manually published 22.5.0 now |
Hello, we tried this new rule but it's failing on dynamic requires. Can you please check it? Thanks! :) #249 |
@thymikee You can cross this off your todo list ;)
@SimenB requesting review :D
Alternative to using
path
would have been/(\/|^)__mocks__(\/|$)/
, but I found this easier to read.Potential config option would be
allowImportBetweenMocks
that disables the rule inside of mocks (IIRC ESLint can give you the path of the file being linted) so you can compose mocks, but I don't consider that to be a good pattern usually so I think you should eslint-disable if you really want to do that.BREAKING CHANGE:
Add no-mocks-import to recommended rules